fix(server): close expired dashboard selection streams quietly when no frames are queued - #4095
Conversation
…o frames are queued The account-selection SSE stream errored with a DOMException on every post-revocation send, including the 15s heartbeat of an expired dashboard session, and Bun prints each errored response stream to the server console. error() exists to discard frames queued before revocation so a revoked consumer cannot drain them; that contract is preserved for a non-empty queue. When the queue is empty (the common expiry path) the stream now closes quietly: pending reads resolve done and there is no error to dump. The two liveness tests flip to the quiet-close expectation, and a new negative test proves queued frames are still discarded on revocation. Closes #4069
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughThe account-selection stream now closes quietly after session expiry when no frame is queued. It reports ChangesAccount stream revocation handling
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to Expired dashboard account-selection streams now close quietly when empty, while queued updates are still discarded after revocation. The covered behavior is ready to merge. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@/home/box/developer/review-comments/pr-4095.md |
|
✅ Deterministic PR hygiene checks passed. |
리뷰 · 우선순위 65 / 80이 PR은 대시보드 관리 세션이 만료된 뒤에도 account-selection SSE가 계속 라인 src/server/management/account-selection-stream.ts · STREAM_QUEUE_HIGH_WATER_MARK - highWaterMark와 판정 상수를 하나로 묶었습니다. 나중에 숫자만 바꾸면 빈 큐를 오판하던 위험을 막습니다. 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Reviewed 589daec against its merge base 8026405. Approving the scoped stream-close fix.
The distinction is correct for this default count-based ReadableStream strategy: desiredSize equals the shared watermark when no chunks are queued. A revoked empty stream closes without emitting another frame; a revoked non-empty stream still errors before cleanup, which discards the queued payload. The new queued-frame negative control checks reader rejection, while pending-read/heartbeat controls check done=true. Admission and the per-send authorization callback are unchanged, and cleanup still removes the subscription, interval, abort listener and connection entry.
I traced the dashboard reader: clean EOF and read errors converge on the same finally/backoff path, so the quiet close does not remove reconnect behavior. This is not a redesign or permission to skip the revocation check.
Exact-head repository CI 34323955157 completed successfully: 19 successful jobs, 2 skipped jobs (macos control and the Windows shard matrix). Gates and Linux/macOS test shards passed. The skipped Windows matrix is not being represented as Windows full-suite evidence. No local product test or live session manipulation was performed.
Scoped security review: the queued-data discard rule on observed revocation is retained; the patch only suppresses an expected error when there is no queued payload to discard. This is a human-reviewed merge candidate, subject to final repository checks. No merge or deployment was performed by this review.
|
Maintainer integration into dev at exact head |
2 participants
Summary
Closes #4069.
Since v2.47.0, an expired dashboard session left the account-selection event stream erroring with a
NotAllowedErrorDOMException on its next send — including the 15-second heartbeat — and Bun prints every errored response stream to the server console, so the console filled with repeated DOMException dumps.controller.error()is not incidental: it is the mechanism that discards frames already queued before revocation, so a revoked consumer cannot drain them. That contract is preserved. The stream now discriminates on queue state: with a non-empty queue it still errors and discards; with an empty queue — the common expired-session path, including every post-expiry heartbeat — it closes quietly, pending reads resolvedone, and there is no error for Bun to dump.The dashboard consumer treats a clean EOF and a stream error identically (same retry-with-backoff path, then quiet 401 on re-admission), so reconnect behavior is unchanged. Admission 401 and per-frame revalidation are unchanged.
The queue watermark is a named constant shared by the stream options and the check, so a future watermark change cannot mis-classify a non-empty queue.
Security review: this touches the management-session revocation path (src/server/management/). The revocation discard contract is preserved and now directly test-covered; per MAINTAINERS.md this still warrants explicit security review before merge.
Verification
dev8026405.Checklist